feat: disables features not needed for Istio deployments #96

bartoszmajsak · 2023-04-17T14:21:23Z

Description

In order to handle the creation of Notebooks that should belong to the service mesh we have to ensure this controller does not create unnecessary resources. For this purpose, when opendatahub.io/service-mesh annotation is present and set to true on the Notebook resource it will make sure that:

no OAuth Proxy related resources are created
no routes are created

It will fail the admission of a notebook pod for the injection of proxy sidecars if annotation opendatahub.io/service-mesh is present and set to true.

I made a few minor adjustments in the test code as I needed to re-use several resources, such as NetworkPolicy.

How Has This Been Tested?

make test to run integration tests
e2e tests can be executed using the following commands:

export K8S_NAMESPACE=opendatahub
kubectl create ns $K8S_NAMESPACE
make deploy-service-mesh 
make deploy-with-mesh -e IMG=quay.io/maistra-dev/odh-notebook-controller -e TAG=enable_ossm
make e2e-test-service-mesh
make undeploy-with-mesh undeploy-service-mesh

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

openshift-ci · 2023-04-17T14:21:35Z

Hi @bartoszmajsak. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bartoszmajsak · 2023-05-11T17:12:48Z

@harshad16 @LaVLaS could you please take a look when you find some time? Many thanks!

VaishnaviHire · 2023-05-15T16:27:54Z

@bartoszmajsak Can we update the e2e tests to capture the changes included in this PR?

bartoszmajsak · 2023-05-15T16:44:03Z

@bartoszmajsak Can we update the e2e tests to capture the changes included in this PR?

Thanks for looking at it @VaishnaviHire. I will update e2e tests as well. Can we ok-to-test in the meanwhile so I can see if there's anything else I overlooked?

VaishnaviHire · 2023-05-15T16:49:05Z

/ok-to-test

bartoszmajsak · 2023-06-05T07:48:15Z

components/notebook-controller/config/overlays/service-mesh/cert-secret.yaml

This is just for testing purposes - not sure if that's the place it should be put. If not I can move it somewhere else and adjust the build process.

components/notebook-controller/config/base/kustomization.yaml

bartoszmajsak · 2023-06-05T07:50:22Z

components/notebook-controller/config/overlays/service-mesh/gateway-route.yaml

+    maistra.io/gateway-name: odh-gateway
+    maistra.io/gateway-namespace: odh-notebook-controller-system
+spec:
+  host: opendatahub.apps-crc.testing


This is replaced on the fly when executing make setup-service-mesh target.

bartoszmajsak · 2023-06-05T07:51:04Z

components/notebook-controller/config/overlays/service-mesh/gateway.yaml

+      protocol: HTTPS
+    tls:
+      mode: SIMPLE
+      credentialName: odh-dashboard-cert


If the overlays here are used mainly for testing purposes I can adjust tests to be using http if desired.

bartoszmajsak · 2023-06-05T07:52:57Z

components/odh-notebook-controller/Makefile

 .PHONY: e2e-test
-e2e-test: ## Run e2e tests for the controller
-	go test ./e2e/ -run ^TestE2ENotebookController -v --nb-namespace=${K8S_NAMESPACE} ${E2E_TEST_FLAGS}
+e2e-test: e2e-test-oauth ## Run e2e tests for the controller with oauth proxy


I decided to keep existing target behavior - to run only tests scenarios where oauth-proxy approach is defined. Happy to adjust based on your needs for CI. There's already a script in place to run the service mesh scenario https://github.com/opendatahub-io/kubeflow/pull/96/files#diff-13d60fc682972fcc720de0f06e1826afc91ba40af5d5c8bd987b370d44d0a332

components/odh-notebook-controller/config/base/kustomization.yaml

bartoszmajsak · 2023-06-05T07:57:12Z

components/odh-notebook-controller/e2e/notebook_controller_test.go

-			// Verify if the configmap has key ISTIO_GATEWAY with value 'kubeflow/kubeflow-gateway'
-			require.EqualValuesf(t, "kubeflow/kubeflow-gateway",
-				configmap.Data["ISTIO_GATEWAY"], "error getting ISTIO_GATEWAY in the configmap")
+			break


I would like to suggest here checking only for the config map existence, as its values can differ by scenario. If the config map is misconfigured the test will fail so that should be enough for end-to-end verification in my eyes.

bartoszmajsak · 2023-06-05T12:26:00Z

@VaishnaviHire I added e2e tests, please let me know if anything else is pending on my end.

In order to handle creation of Notebooks which should belong to the service mesh we have to ensure this controller does not create unnecessary resources. For this purpose, when opendatahub.io/service-mesh annotation is present on the Notebook resource it will make sure that: - no OAuth Proxy related resources are created - no routes are created It will fail admission of a notebook pod for the injection of proxy sidecars if annotation opendatahub.io/service-mesh is present and set to true as well. I made few minor adjustements in the test code as I needed to re-use several resources, such as NetworkPolicy.

One reason being IDEs such as GoLand have troubles recongizing private funcs, but it is mainly due to the fact that it is a purely test code

…nario

VaishnaviHire · 2023-06-14T12:06:07Z

@bartoszmajsak Overall code changes look good to me. On the local test run I am seeing following error when creating Notebook instance

Error creating Notebook resource thoth-minimal-service-mesh-notebook: Internal error occurred: failed calling webhook "notebooks.opendatahub.io": failed to call webhook: Post "https://odh-notebook-controller-webhook-service.opendatahub.svc:443/mutate-notebook-v1?timeout=10s": context deadline exceeded, trying again

Did you come across this while testing?

Note that I am running the tests in the opendatahub namespaces

bartoszmajsak · 2023-06-14T14:12:27Z

@bartoszmajsak Overall code changes look good to me. On the local test run I am seeing following error when creating Notebook instance
Error creating Notebook resource thoth-minimal-service-mesh-notebook: Internal error occurred: failed calling webhook "notebooks.opendatahub.io": failed to call webhook: Post "https://odh-notebook-controller-webhook-service.opendatahub.svc:443/mutate-notebook-v1?timeout=10s": context deadline exceeded, trying again
Did you come across this while testing?

Note that I am running the tests in the opendatahub namespaces

@VaishnaviHire Leaving aside the fact that Makefile is not propagating the ns correctly between the steps (it's using default odh-notebook-controller-system ns regardless) there might be one related problem. I ran those tests on CRC and I noticed that sometimes there was an issue with istio-cni which was leading to the notebook pod stuck in ContainerCreating state. The solution was to simply restart istio-cni pod - see https://access.redhat.com/solutions/6995845. You can check if you see cni-related events in the openshift console for example.

I will fix the ns propagation.

bartoszmajsak · 2023-06-15T03:57:39Z

@VaishnaviHire I prepared a similar bash script for ci tests - https://github.com/opendatahub-io/kubeflow/pull/96/files#diff-13d60fc682972fcc720de0f06e1826afc91ba40af5d5c8bd987b370d44d0a332

Should this be incorporated as part of this work?

bartoszmajsak · 2023-06-21T18:28:14Z

@VaishnaviHire @harshad16 Could someone please give it another look?

shalberd · 2023-06-23T06:22:30Z

that sometimes there was an issue with istio-cni which was leading to the notebook pod stuck in ContainerCreating state

Yeah, Istio can be a bit finicky :-)

@bartoszmajsak excellent work, and a big requirement for i.e. using an external auth provider like IBMAppID in conjunction with Istio / ServiceMesh, as well as for support for evaluating RBAC via the kubeflow profiles controller medium-term. Psyched to see your contribution. Wielkie dzię and merci vielmal; best regards from Lake Lauerz.

bartoszmajsak · 2023-06-26T12:13:12Z

@bartoszmajsak excellent work, and a big requirement for i.e. using an external auth provider like IBMAppID in conjunction with Istio / ServiceMesh, as well as for support for evaluating RBAC via the kubeflow profiles controller medium-term. Psyched to see your contribution. Wielkie dzię and merci vielmal; best regards from Lake Lauerz.

Thanks @shalberd! Also for pointing out the profile controller - I will take a closer look to see how we can leverage it. Lake Lauerz is great - I must return there for SUP this summer!

shalberd · 2023-06-27T14:17:41Z

@bartoszmajsak if you ever want to connect with IBM's Sebastian Lehrig @lehrig, he's got heaps of experience making Kubeflow, including RBAC with Kubeflow Profiles controller, work on Openshift (within a PowerPC initiative). @raffaelespazzoli worked in detail on it, being the detective on Openshift

lehrig · 2023-06-27T18:15:18Z

thanks for the flowers @shalberd - kudos particularly goes to Raffaele who did lots of the ground work: https://cloud.redhat.com/blog/operationalizing-kubeflow-in-openshift

harshad16

This looks good to me.
i had a little trouble installing , when tried to do re-install
however i see that was due to my cluster.

/lgtm
/approve

thanks 👍

openshift-ci · 2023-07-05T13:37:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [harshad16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bartoszmajsak · 2023-07-05T15:37:41Z

i had a little trouble installing , when tried to do re-install
however i see that was due to my cluster.

Thanks for taking care of it @harshad16. If you still have some details about the failures, such as logs, I can have a look and think about how this could be improved.

brings back opendatahub-io#96

openshift-ci bot added do-not-merge/work-in-progress needs-ok-to-test labels Apr 17, 2023

bartoszmajsak changed the title ~~feat: disables features conflicting with Istio~~ feat: disables features not need for Istio deployments Apr 17, 2023

bartoszmajsak mentioned this pull request Apr 17, 2023

Enabling Service Mesh opendatahub-io/odh-model-controller#23

Closed

bartoszmajsak marked this pull request as ready for review April 19, 2023 12:47

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 19, 2023

openshift-ci bot requested review from harshad16 and LaVLaS April 19, 2023 12:47

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels May 15, 2023

bartoszmajsak commented Jun 5, 2023

View reviewed changes

components/notebook-controller/config/base/kustomization.yaml Outdated Show resolved Hide resolved

bartoszmajsak commented Jun 5, 2023

View reviewed changes

components/odh-notebook-controller/config/base/kustomization.yaml Outdated Show resolved Hide resolved

bartoszmajsak commented Jun 5, 2023

View reviewed changes

bartoszmajsak added 7 commits June 5, 2023 17:03

chore: renames helper to be part of _test

7f83b00

One reason being IDEs such as GoLand have troubles recongizing private funcs, but it is mainly due to the fact that it is a purely test code

chore: switches to os.ReadFile from deprecated ioutil counterpart

ce4bfe6

feat: enables test filtering for OSSM and OAuth scenarios

9d6774e

feat(e2e): introduces cli flag to run selected type of e2e tests

20f39d1

feat: introduces sevice-mesh e2e tests

03539ce

feat(e2e): tests for relevant network policy creation and deletion

59041b2

bartoszmajsak added 2 commits June 5, 2023 17:03

chore: improves readibility of assertions depending on deployment sce…

28ba6bc

…nario

chore: reverts image tags

3fead38

bartoszmajsak force-pushed the enable_ossm branch from ccc8e3c to 3fead38 Compare June 5, 2023 15:03

chore(make): allows to set K8S_NS as env variable

9a88950

bartoszmajsak changed the title ~~feat: disables features not need for Istio deployments~~ feat: disables features not needed for Istio deployments Jun 23, 2023

harshad16 approved these changes Jul 5, 2023

View reviewed changes

openshift-ci bot assigned harshad16 Jul 5, 2023

openshift-ci bot added the lgtm label Jul 5, 2023

openshift-ci bot added the approved label Jul 5, 2023

openshift-merge-robot merged commit 92739d6 into opendatahub-io:v1.6-branch Jul 5, 2023

bartoszmajsak deleted the enable_ossm branch July 5, 2023 15:36

harshad16 mentioned this pull request Jul 13, 2023

Setup OSSM on odh-notebook-controller #103

Closed

This was referenced Sep 6, 2023

Service Mesh SSO(Auth) Integration opendatahub-io/opendatahub-community#97

Open

v1.7 branch is missing Service Mesh logic for handling notebooks #168

Closed

bartoszmajsak added a commit to bartoszmajsak/odh-kubeflow that referenced this pull request Sep 11, 2023

feat: disables features not needed for Istio deployments

8aea11e

brings back opendatahub-io#96

bartoszmajsak mentioned this pull request Sep 11, 2023

feat: disables features not needed for Istio deployments #172

Merged

3 tasks

bartoszmajsak added a commit to bartoszmajsak/odh-kubeflow that referenced this pull request Oct 3, 2023

feat: disables features not needed for Istio deployments

a2de58e

brings back opendatahub-io#96

bartoszmajsak mentioned this pull request Oct 6, 2023

Application namespace hardcoded for Kubeflow opendatahub-io/opendatahub-operator#617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: disables features not needed for Istio deployments #96

feat: disables features not needed for Istio deployments #96

bartoszmajsak commented Apr 17, 2023 •

edited

Loading

openshift-ci bot commented Apr 17, 2023

bartoszmajsak commented May 11, 2023

VaishnaviHire commented May 15, 2023

bartoszmajsak commented May 15, 2023

VaishnaviHire commented May 15, 2023

bartoszmajsak Jun 5, 2023

bartoszmajsak Jun 5, 2023

bartoszmajsak Jun 5, 2023

bartoszmajsak Jun 5, 2023

bartoszmajsak Jun 5, 2023

bartoszmajsak commented Jun 5, 2023

VaishnaviHire commented Jun 14, 2023 •

edited

Loading

bartoszmajsak commented Jun 14, 2023 •

edited

Loading

bartoszmajsak commented Jun 15, 2023 •

edited

Loading

bartoszmajsak commented Jun 21, 2023

shalberd commented Jun 23, 2023 •

edited

Loading

bartoszmajsak commented Jun 26, 2023

shalberd commented Jun 27, 2023 •

edited

Loading

lehrig commented Jun 27, 2023

harshad16 left a comment

openshift-ci bot commented Jul 5, 2023

bartoszmajsak commented Jul 5, 2023

feat: disables features not needed for Istio deployments #96

feat: disables features not needed for Istio deployments #96

Conversation

bartoszmajsak commented Apr 17, 2023 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

openshift-ci bot commented Apr 17, 2023

bartoszmajsak commented May 11, 2023

VaishnaviHire commented May 15, 2023

bartoszmajsak commented May 15, 2023

VaishnaviHire commented May 15, 2023

bartoszmajsak Jun 5, 2023

Choose a reason for hiding this comment

bartoszmajsak Jun 5, 2023

Choose a reason for hiding this comment

bartoszmajsak Jun 5, 2023

Choose a reason for hiding this comment

bartoszmajsak Jun 5, 2023

Choose a reason for hiding this comment

bartoszmajsak Jun 5, 2023

Choose a reason for hiding this comment

bartoszmajsak commented Jun 5, 2023

VaishnaviHire commented Jun 14, 2023 • edited Loading

bartoszmajsak commented Jun 14, 2023 • edited Loading

bartoszmajsak commented Jun 15, 2023 • edited Loading

bartoszmajsak commented Jun 21, 2023

shalberd commented Jun 23, 2023 • edited Loading

bartoszmajsak commented Jun 26, 2023

shalberd commented Jun 27, 2023 • edited Loading

lehrig commented Jun 27, 2023

harshad16 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 5, 2023

bartoszmajsak commented Jul 5, 2023

bartoszmajsak commented Apr 17, 2023 •

edited

Loading

VaishnaviHire commented Jun 14, 2023 •

edited

Loading

bartoszmajsak commented Jun 14, 2023 •

edited

Loading

bartoszmajsak commented Jun 15, 2023 •

edited

Loading

shalberd commented Jun 23, 2023 •

edited

Loading

shalberd commented Jun 27, 2023 •

edited

Loading